Closed Bug 1516277 Opened 6 years ago Closed 6 years ago

Hit MOZ_CRASH(This is unsafe! Fix the caller!) at src/dom/events/EventDispatcher.cpp:818

Categories

(Core :: Storage: localStorage & sessionStorage, defect, P2)

Product:
Core
Component:
Storage: localStorage & sessionStorage
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox65 --- disabled
firefox66 + disabled
firefox67 + verified

People

(Reporter: tsmith, Assigned: ytausky)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [post-critsmash-triage])

Attachments

(3 files)

testcase.html
186 bytes, text/html
Details
WIP
2.72 KB, patch
Details | Diff | Splinter Review
Add local execution mode to nsThread
47 bytes, text/x-phabricator-request
Details | Review
Attached file testcase.html —
Could this be related to (or a dup of) bug 1516018? They appeared around the same time. Hit MOZ_CRASH(This is unsafe! Fix the caller!) at src/dom/events/EventDispatcher.cpp:818 #0 mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) src/dom/events/EventDispatcher.cpp:1079:5 #1 mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsPresContext*, nsEventStatus*) src/dom/events/EventDispatcher.cpp #2 nsINode::DispatchEvent(mozilla::dom::Event&, mozilla::dom::CallerType, mozilla::ErrorResult&) src/dom/base/nsINode.cpp:1029:17 #3 nsContentUtils::DispatchEvent(nsIDocument*, nsISupports*, nsTSubstring<char16_t> const&, mozilla::CanBubble, mozilla::Cancelable, mozilla::Composed, mozilla::Trusted, bool*, mozilla::ChromeOnlyDispatch) src/dom/base/nsContentUtils.cpp:4065:28 #4 nsContentUtils::DispatchTrustedEvent(nsIDocument*, nsISupports*, nsTSubstring<char16_t> const&, mozilla::CanBubble, mozilla::Cancelable, mozilla::Composed, bool*) src/dom/base/nsContentUtils.cpp:4035:10 #5 mozilla::css::SheetLoadData::FireLoadEvent(nsIThreadInternal*) src/layout/style/Loader.cpp:307:3 #6 mozilla::css::SheetLoadData::OnProcessNextEvent(nsIThreadInternal*, bool) src/layout/style/Loader.cpp:283:3 #7 non-virtual thunk to mozilla::css::SheetLoadData::OnProcessNextEvent(nsIThreadInternal*, bool) src/layout/style/Loader.cpp #8 nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1068:3 #9 NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:468:10 #10 bool mozilla::SpinEventLoopUntil<(mozilla::ProcessFailureBehavior)1, mozilla::dom::(anonymous namespace)::RequestHelper::StartAndReturnResponse(mozilla::dom::LSRequestResponse&)::$_7>(mozilla::dom::(anonymous namespace)::RequestHelper::StartAndReturnResponse(mozilla::dom::LSRequestResponse&)::$_7&&, nsIThread*) src/obj-firefox/dist/include/nsThreadUtils.h:335:25 #11 mozilla::dom::(anonymous namespace)::RequestHelper::StartAndReturnResponse(mozilla::dom::LSRequestResponse&) src/dom/localstorage/LSObject.cpp:1017:7 #12 mozilla::dom::LSObject::DoRequestSynchronously(mozilla::dom::LSRequestParams const&, mozilla::dom::LSRequestResponse&) src/dom/localstorage/LSObject.cpp:695:25 #13 mozilla::dom::LSObject::EnsureObserver() src/dom/localstorage/LSObject.cpp:808:17 #14 nsGlobalWindowInner::EventListenerAdded(nsAtom*) src/dom/base/nsGlobalWindowInner.cpp:6180:17 #15 mozilla::EventListenerManager::AddEventListenerInternal(mozilla::dom::CallbackObjectHolder<mozilla::dom::EventListener, nsIDOMEventListener>, mozilla::EventMessage, nsAtom*, mozilla::EventListenerFlags const&, bool, bool) src/dom/events/EventListenerManager.cpp:414:14 #16 mozilla::EventListenerManager::SetEventHandlerInternal(nsAtom*, mozilla::TypedEventHandler const&, bool) src/dom/events/EventListenerManager.cpp:733:5 #17 mozilla::EventListenerManager::SetEventHandler(nsAtom*, nsTSubstring<char16_t> const&, bool, bool, mozilla::dom::Element*) src/dom/events/EventListenerManager.cpp:830:24 #18 mozilla::dom::Element::SetEventHandler(nsAtom*, nsTSubstring<char16_t> const&, bool) src/dom/base/Element.cpp:2226:12 #19 nsGenericHTMLElement::AfterSetAttr(int, nsAtom*, nsAttrValue const*, nsAttrValue const*, nsIPrincipal*, bool) src/dom/html/nsGenericHTMLElement.cpp:597:21 #20 mozilla::dom::HTMLBodyElement::AfterSetAttr(int, nsAtom*, nsAttrValue const*, nsAttrValue const*, nsIPrincipal*, bool) src/dom/html/HTMLBodyElement.cpp:301:39 #21 mozilla::dom::Element::SetAttrAndNotify(int, nsAtom*, nsAtom*, nsAttrValue const*, nsAttrValue&, nsIPrincipal*, unsigned char, bool, bool, bool, nsIDocument*, mozAutoDocUpdate const&) src/dom/base/Element.cpp:2536:10 #22 mozilla::dom::Element::SetAttr(int, nsAtom*, nsAtom*, nsTSubstring<char16_t> const&, nsIPrincipal*, bool) src/dom/base/Element.cpp:2399:10 #23 mozilla::dom::Element::SetAttr(int, nsAtom*, nsTSubstring<char16_t> const&, nsIPrincipal*, bool) src/obj-firefox/dist/include/mozilla/dom/Element.h:840:12 #24 mozilla::dom::Element::SetAttribute(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, nsIPrincipal*, mozilla::ErrorResult&) src/dom/base/Element.cpp:1369:14 #25 mozilla::dom::Element_Binding::setAttribute(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitMethodCallArgs const&) src/obj-firefox/dom/bindings/ElementBinding.cpp:1330:9 #26 bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) src/dom/bindings/BindingUtils.cpp:3062:13 #27 CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) src/js/src/vm/Interpreter.cpp:443:13 #28 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:535:12 #29 InternalCall(JSContext*, js::AnyInvokeArgs const&) src/js/src/vm/Interpreter.cpp:590:10 #30 Interpret(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:3320:16 #31 js::RunScript(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:423:10 #32 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:563:13 #33 InternalCall(JSContext*, js::AnyInvokeArgs const&) src/js/src/vm/Interpreter.cpp:590:10 #34 js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) src/js/src/vm/Interpreter.cpp:606:8 #35 JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) src/js/src/jsapi.cpp:2649:10 #36 mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) src/obj-firefox/dom/bindings/EventHandlerBinding.cpp:265:37 #37 void mozilla::dom::EventHandlerNonNull::Call<nsISupports*>(nsISupports* const&, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*) src/obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:363:12 #38 mozilla::JSEventHandler::HandleEvent(mozilla::dom::Event*) src/dom/events/JSEventHandler.cpp:205:12 #39 mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) src/dom/events/EventListenerManager.cpp:1043:51 #40 mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) src/dom/events/EventListenerManager.cpp:1238:17 #41 mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:350:17 #42 mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:552:16 #43 mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) src/dom/events/EventDispatcher.cpp:1042:11 #44 nsDocumentViewer::LoadComplete(nsresult) src/layout/base/nsDocumentViewer.cpp:1103:7 #45 nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) src/docshell/base/nsDocShell.cpp:6709:21 #46 nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) src/docshell/base/nsDocShell.cpp:6513:7 #47 non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) src/docshell/base/nsDocShell.cpp #48 nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) src/uriloader/base/nsDocLoader.cpp:1235:3 #49 nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) src/uriloader/base/nsDocLoader.cpp:794:14 #50 nsDocLoader::DocLoaderIsEmpty(bool) src/uriloader/base/nsDocLoader.cpp:693:9 #51 nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) src/uriloader/base/nsDocLoader.cpp:589:5 #52 non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) src/uriloader/base/nsDocLoader.cpp #53 mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) src/netwerk/base/nsLoadGroup.cpp:579:22 #54 nsIDocument::DoUnblockOnload() src/dom/base/nsDocument.cpp:7728:18 #55 nsDocument::UnblockOnload(bool) src/dom/base/nsDocument.cpp:7660:9 #56 nsIDocument::DispatchContentLoadedEvents() src/dom/base/nsDocument.cpp:4829:3 #57 mozilla::detail::RunnableMethodImpl<nsIDocument*, void (nsIDocument::*)(), true, (mozilla::RunnableKind)0>::Run() src/obj-firefox/dist/include/nsThreadUtils.h:1158:13 #58 mozilla::SchedulerGroup::Runnable::Run() src/xpcom/threads/SchedulerGroup.cpp:299:32 #59 nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1157:14 #60 NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:468:10 #61 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:88:21 #62 MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:314:10 #63 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:289:3 #64 nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:137:27 #65 XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:915:20 #66 mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:238:9 #67 MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:314:10 #68 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:289:3 #69 XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:753:34 #70 content_process_main(mozilla::Bootstrap*, int, char**) src/browser/app/../../ipc/contentproc/plugin-container.cpp:49:28 #71 main src/browser/app/nsBrowserApp.cpp:265:18 #72 __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291 #73 _start (firefox+0x349f4)
Flags: in-testsuite?
mozilla::SpinEventLoopUntil in middle of a DOM operation. Really bad.
Blocks: 1286798
Component: DOM: Events → DOM: Web Storage
LSNG creates a nested event queue (by calling PushEventQueue) before calling mozilla::SpinEventLoopUntil, so nothing else should be processed. Maybe that's not always true on the main thread. This comment looks suspicious: https://searchfox.org/mozilla-central/rev/3e7afaa5b57b3f8ed100cd1f13bb0a93b9b2cb99/xpcom/threads/nsThreadUtils.h#326
No, it's a thread observer that causes this crash.
(In reply to Jan Varga [:janv] from comment #2) > LSNG creates a nested event queue (by calling PushEventQueue) before calling > mozilla::SpinEventLoopUntil, so nothing else should be processed. > > Maybe that's not always true on the main thread. PushEventQueue only does what you want it to do on workers, fwiw.
(In reply to Nathan Froyd [:froydnj] from comment #4) > (In reply to Jan Varga [:janv] from comment #2) > > LSNG creates a nested event queue (by calling PushEventQueue) before calling > > mozilla::SpinEventLoopUntil, so nothing else should be processed. > > > > Maybe that's not always true on the main thread. > > PushEventQueue only does what you want it to do on workers, fwiw. Ok, thread observers don't expect a nested event queue to exist, for example: https://searchfox.org/mozilla-central/rev/3e7afaa5b57b3f8ed100cd1f13bb0a93b9b2cb99/layout/style/Loader.cpp#279 That looks fixable, do you know if there's anything else ?
See Also: → 1516282
Keywords: sec-high

After some discussion, it looks like the best option here would be add another interface for background child actor initialization. The new interface would return a runnable than must be executed on the main thread, so it can either get dispatched on it (that's what GetOrCreateForCurrentThread does), or sent by other means. In LS's case, we'd send it over directly and run it inside StartAndReturnResponse, thus avoiding the deadlock problem that required the event loop spinning.

Assignee: nobody → ytausky
Priority: -- → P2
Attached patch WIP — — Splinter Review

The right approach seems to be a contentious issue, I'll update here once we reach consensus.

After further discussion, I'm going to go ahead with the approach outlined by Jan.

[Tracking Requested - why for this release]: sec-high found by both internal and external fuzzing (bug 1516018).

tracking-firefox66: --- → ?

Short update: since we decided to stick with the nested event loops, I've been looking at what's required to support them. The stack in this bug is reached by a thread observer that's installed by the CSS loader in what seems to be somewhat of a hack. I'm currently working on implementing it with normal primitives instead.

Just a note: the code path the hits this bug is currently preffed off by default (it's part of LSNG).

(In reply to Yaron Tausky [:ytausky] from comment #13)

Just a note: the code path the hits this bug is currently preffed off by default (it's part of LSNG).

Does that mean we don't need to fix this in 66 (Liz is tracking this (see above between comment 11 and comment 12))?

Yes, I don't think this is a must-fix for 66. The only way a user could be affected is by them turning LSNG on in about:config.

Yaron is doing some work in bug 1523581 related to this.

Landed: https://hg.mozilla.org/integration/autoland/rev/44d83d32e254ccdb9441b8c0388262e436918da5

Backed out changeset 44d83d32e254 (bug 1516277) for failing at /test/unit/test_eviction.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/4e12cf72f03b56184aacaef83af7719565eafce3

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&selectedJob=232175644&revision=44d83d32e254ccdb9441b8c0388262e436918da5

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=232175644&repo=autoland&lineNumber=2121

Log snippet:

[task 2019-03-06T15:24:04.859Z] 15:24:04 INFO - TEST-START | dom/localstorage/test/unit/test_eviction.js
[task 2019-03-06T15:24:05.152Z] 15:24:05 WARNING - TEST-UNEXPECTED-FAIL | dom/localstorage/test/unit/test_eviction.js | xpcshell return code: 0
[task 2019-03-06T15:24:05.152Z] 15:24:05 INFO - TEST-INFO took 292ms
[task 2019-03-06T15:24:05.153Z] 15:24:05 INFO - >>>>>>>
[task 2019-03-06T15:24:05.154Z] 15:24:05 INFO - (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2019-03-06T15:24:05.155Z] 15:24:05 INFO - TEST-PASS | dom/localstorage/test/unit/test_eviction.js | undefined assertion name - There should be a testSteps function - true == true
[task 2019-03-06T15:24:05.156Z] 15:24:05 INFO - TEST-PASS | dom/localstorage/test/unit/test_eviction.js | undefined assertion name - testSteps should be an async function - true == true
[task 2019-03-06T15:24:05.157Z] 15:24:05 INFO - (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2019-03-06T15:24:05.157Z] 15:24:05 INFO - (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2019-03-06T15:24:05.159Z] 15:24:05 INFO - running event loop
[task 2019-03-06T15:24:05.159Z] 15:24:05 INFO - "CONSOLE_MESSAGE: (info) No chrome package registered for chrome://branding/locale/brand.properties"
[task 2019-03-06T15:24:05.163Z] 15:24:05 INFO - dom/localstorage/test/unit/test_eviction.js | Starting testSteps
[task 2019-03-06T15:24:05.164Z] 15:24:05 INFO - (xpcshell/head.js) | test testSteps pending (2)
[task 2019-03-06T15:24:05.164Z] 15:24:05 INFO - "Setting prefs"
[task 2019-03-06T15:24:05.167Z] 15:24:05 INFO - "Setting limits"
[task 2019-03-06T15:24:05.167Z] 15:24:05 INFO - (xpcshell/head.js) | test run_next_test 0 finished (2)
[task 2019-03-06T15:24:05.169Z] 15:24:05 INFO - "Getting storages"
[task 2019-03-06T15:24:05.172Z] 15:24:05 INFO - "Filling up entire default storage"
[task 2019-03-06T15:24:05.172Z] 15:24:05 INFO - Unexpected exception QuotaExceededError: The quota has been exceeded.
[task 2019-03-06T15:24:05.172Z] 15:24:05 INFO - testSteps@/builds/worker/workspace/build/tests/xpcshell/tests/dom/localstorage/test/unit/test_eviction.js:42:17
[task 2019-03-06T15:24:05.172Z] 15:24:05 INFO - async*run_next_test/_run_next_test/<@/builds/worker/workspace/build/tests/xpcshell/head.js:1434:22
[task 2019-03-06T15:24:05.173Z] 15:24:05 INFO - _run_next_test@/builds/worker/workspace/build/tests/xpcshell/head.js:1434:38
[task 2019-03-06T15:24:05.173Z] 15:24:05 INFO - run@/builds/worker/workspace/build/tests/xpcshell/head.js:685:9
[task 2019-03-06T15:24:05.174Z] 15:24:05 INFO - _do_main@/builds/worker/workspace/build/tests/xpcshell/head.js:224:6
[task 2019-03-06T15:24:05.175Z] 15:24:05 INFO - _execute_test@/builds/worker/workspace/build/tests/xpcshell/head.js:526:5
[task 2019-03-06T15:24:05.175Z] 15:24:05 INFO - @-e:1:1
[task 2019-03-06T15:24:05.176Z] 15:24:05 INFO - exiting test

Flags: needinfo?(ytausky)

The test needed to be fixed, but I thought the fix already landed. I'll include it in the next landing attempt.

Flags: needinfo?(ytausky)

https://hg.mozilla.org/integration/autoland/rev/29e28daebae57e16db9d32017e7f34eb968d02fb
https://hg.mozilla.org/mozilla-central/rev/29e28daebae5

Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]

I verified the fix using the test case attached on asan builds on Ubuntu 18.04 x64 and Windows 10 x64. The issue is not reproducing.
However, I couldn't reproduce the issue on an older build because the flag for Fx 66 and Fx 55 is disabled. Can somebody that could reproduce the issue verify the fix also? Just to be sure that everything is fine.
Thank you.

Flags: needinfo?(twsmith)

Verified fixed on 67 branch.

Flags: needinfo?(twsmith)

Taking in consideration what is written in comment 21 and comment 22 I will mark this bug as verified fixed.

Status: RESOLVED → VERIFIED
status-firefox67: fixed → verified
Flags: qe-verify+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: